Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

link: stop unneeded force linking on Mojave/CLT 10. #4441

Merged
merged 1 commit into from
Jul 9, 2018
Merged

link: stop unneeded force linking on Mojave/CLT 10. #4441

merged 1 commit into from
Jul 9, 2018

Conversation

MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Jul 9, 2018 •
edited
Loading

People are getting in the habit of force-linking things like zlib to fix linking/include issues on Mojave (which doesn't install headers to /usr/include by default). This way lies madness so encourage people to instead pass the correct compiler flags instead.

References Homebrew/homebrew-core#28423 (comment)

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

@ghost ghost assigned MikeMcQuaid Jul 9, 2018
@ghost ghost added the in progress Maintainers are working on this label Jul 9, 2018
@MikeMcQuaid MikeMcQuaid changed the title ignlink: stop unneeded force linking on Xcode 10/Mojave. ignlink: stop unneeded force linking on Mojave/CLT 10. Jul 9, 2018
@MikeMcQuaid MikeMcQuaid requested a review from ilovezfs July 9, 2018 14:41
@MikeMcQuaid MikeMcQuaid changed the title ignlink: stop unneeded force linking on Mojave/CLT 10. link: stop unneeded force linking on Mojave/CLT 10. Jul 9, 2018
@MikeMcQuaid
link: stop unneeded force linking on Mojave/CLT 10.
2f181b3 
People are getting in the habit of force-linking things like `zlib` to
fix linking/include issues on Mojave (which doesn't install headers to
`/usr/include` by default). This way lies madness so encourage people to
instead pass the correct compiler flags instead.
ilovezfs
ilovezfs reviewed Jul 9, 2018
View reviewed changes
Library/Homebrew/cmd/link.rb
elsif (MacOS.version >= :mojave ||
MacOS::Xcode.version >= "10.0" ||
MacOS::CLT.version >= "10.0") &&
dep_f.keg_only_reason.reason == :provided_by_macos
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we also want to check shadowed_by_macos too since finding the libedit headers may be an issue too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 but let's wait until we see that in the wild first?

@MikeMcQuaid MikeMcQuaid merged commit d445406 into Homebrew:master Jul 9, 2018
@ghost ghost removed the in progress Maintainers are working on this label Jul 9, 2018
@MikeMcQuaid
Copy link
Member Author

Merging as-is for now but plan on adding :shadowed if we see it and perhaps non-Mojave with odeprecated in future 👍

@DomT4
Copy link
Contributor

DomT4 commented Jul 10, 2018

Apparently I can now link keg_only stuff without any kind of warning or needing to pass the --force flag? Not sure whether that's intentional but the documentation would still seem to indicate that it isn't.

~> brew link readline
Linking /usr/local/Cellar/readline/7.0.5... 16 symlinks created

~> ls -1 /usr/local/lib | grep readline
libreadline.7.0.dylib
libreadline.7.dylib
libreadline.a
libreadline.dylib

@ilovezfs
Copy link
Contributor

That's a bug.

@DomT4
Copy link
Contributor

DomT4 commented Jul 10, 2018

Running brew link <xyz> on something that has already been linked no longer produces the correct behaviour either.

@ilovezfs
Copy link
Contributor

yeah the if HOMEBREW_PREFIX.to_s == "/usr/local" && keg_only is catching more than intended

@MikeMcQuaid MikeMcQuaid mentioned this pull request Jul 10, 2018
Merged
6 tasks
@billinghamj
Copy link

So following this change, how are we meant to use things like curl after installing? Previously the only (simple) way to switch the built-in curl command to the new one was to use the link command. Is there another tool brew provides for this purpose?

@DomT4
Copy link
Contributor

DomT4 commented Jul 25, 2018

how are we meant to use things like curl after installing?

If you need to have this software first in your PATH run:
  echo 'export PATH="/usr/local/opt/curl/bin:$PATH"' >> ~/.zshrc

For compilers to find this software you may need to set:
    LDFLAGS:  -L/usr/local/opt/curl/lib
    CPPFLAGS: -I/usr/local/opt/curl/include
For pkg-config to find this software you may need to set:
    PKG_CONFIG_PATH: /usr/local/opt/curl/lib/pkgconfig

brew link is a one-line command and echo 'export blah is a one-line command, so I don't see a major increase in difficulty using curl, personally.

@billinghamj
Copy link

Thanks.

I believe a lot of people were using brew link for this purpose. It might be worth adjusting the refusal message to include the same statement as above "If you need to have this software first in your PATH run" etc

@DomT4
Copy link
Contributor

DomT4 commented Jul 25, 2018

It'd be relatively easy to throw out to Caveats to simply print the caveats again but I don't know how much appetite there is to change this from Mike/ILZ given 1) Anyone running 10.14 or Xcode 10 should theoretically be comfortable enough with software/developer tools/etc to handle this change themselves, 2) This is presumably temporary until 10.14 is stable/GM.

I'll let one of them comment as they wish to before I stick my head into this one too much.

@ilovezfs
Copy link
Contributor

More likely the approach will be applied to <= 10.13 as well, possibly with a deprecation warning first.

@MikeMcQuaid MikeMcQuaid deleted the fewer-mojave-force-links branch July 26, 2018 09:10
@MikeMcQuaid MikeMcQuaid mentioned this pull request Jul 26, 2018
Merged
6 tasks
@lock lock bot added the outdated PR was locked due to age label Aug 25, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ilovezfs ilovezfs ilovezfs left review comments

Assignees

@MikeMcQuaid MikeMcQuaid

Labels
outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MikeMcQuaid @DomT4 @ilovezfs @billinghamj @keyurp1987